Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for building and using Mac Catalyst offsets #52777

Merged
merged 4 commits into from
May 18, 2021

Conversation

directhex
Copy link
Member

No description provided.

@directhex
Copy link
Member Author

directhex commented May 17, 2021

Running a private build since this affects packaging

https://dev.azure.com/dnceng/internal/_build/results?buildId=1141962&view=results

@directhex directhex marked this pull request as ready for review May 18, 2021 13:31
@directhex directhex added the os-maccatalyst MacCatalyst OS label May 18, 2021
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple nits that can be addressed in a follow-up PR.

@@ -893,6 +893,9 @@
/* The JIT/AOT targets tvOS */
#cmakedefine TARGET_TVOS 1

/* The JIT/AOT targets Mac Catalyst */
#cmakedefine TARGET_MACCAT 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we should call this TARGET_MACCATALYST so it's consistent with how we're using it everywhere else (can be done in a follow-up PR).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That came in with #51669, I'm innocent

@@ -208,6 +208,7 @@ jobs:
- Browser_wasm
- tvOS_arm64
- iOS_arm64
- MacCatalyst_x64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if I understand this correctly this job builds the AOT offsets for all archs, not just iOS_arm64 or MacCatalyst_x64 right? we should add a comment to explain it since this is quite confusing.

@directhex directhex merged commit 837605b into dotnet:main May 18, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants